-
-
Notifications
You must be signed in to change notification settings - Fork 525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Binder #2198
Enable Binder #2198
Conversation
Status: Things are starting to work. I'm finding bugs in the Reference Examples. I have to update the gallery notebooks in order to be able to serve them as apps. I would like to configure VS Code to enable showcasing how powerful the combination of Panel and VS Code is.... Also on Binder and in a Jupyter Hub. panel-binder.mp4So I would really, really like Panel to work in VS Code. But it requires fixing bokeh/jupyter_bokeh#131 |
Hi @philippjfr As I see it 1) Binder is working 2) It now comes with a nice setup like In principle this could be all for this PR except adding a link to binder in different places. For example the README. BUT all the notebooks need to be walked through and prettified in order to be useable as apps. This could be a lot of work and a lot of minor changes. And to be honest. Even without binder a test and update of the notebooks could be in place (c.f. all the bugs posted, I have just scratched the surface I believe). Here are my questions.
|
No, some of them sure, but definitely not all. The focus should be the feature being demonstrated not making all the examples look like.
Definitely not, in fact I'd argue there's no reason to make these available as apps at all.
I'll make sure to configure this once you're done here. The link will go in the interactivity warning that floats on the right side. |
Just make a single PR for any coordinated changes like this. |
Binder is now working as I originally intended it to and more :-).
Some things are not in scope for me
Some things are not working and I am not capable of getting it working
Working Videobinder-gallery-works.mp4Not WorkingWatch this video on Youtube https://www.youtube.com/watch?v=gI3qQRVdIIE @philippjfr Feel free to review and start creating value from this :-) I don't plan to do more on this before getting a review and feedback. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This will make things look and feel much better. Should we promote the approach from #2271 or is it better to put the template separately like it is here?
Xvfb :99 -screen 0 1280x1024x24 > /dev/null 2>&1 & | ||
sleep 3 | ||
jupyter trust examples/**/*.ipynb | ||
jupyter trust examples/**/**/*.ipynb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a bash expert, but doesn't **
indicate recursive directory searching nowadays? If so, isn't the second trust call redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so. But it did not work like that for me. So I added it twice. That works.
When the new approach #2271 is ready and tested I think it would make a lot of sense to switch. That syntax is nice and shorter. The only downside is that then not a lot of examples will show how to build apps with templates for non-notebook users. There the natural workflow might still be to explicitly declare a template ?? |
Good point. We should still have a separate example of building such a template explicitly somewhere, and while it's true people may not see that example, having people use servable unnecessarily in a standalone file is not that terrible. I.e., I agree it doesn't make as much sense outside a notebook, but it's still going to be fine for many people, and so I'm ok with more advanced non-notebook users having to do a bit more work to find that there is this other way. |
setup.py
Outdated
'pydeck', | ||
'pyecharts', | ||
'idom', | ||
'xlsxwriter', | ||
'pyarrow', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these cannot be added to setup.py since that is also the ground truth for conda installs. If I remove these here will this break the binder setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But we can add them to one of the files in the binder subfolder. Can you point out which should not be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already removed all the ones you added. Anything that is not conda installable cannot appear here, so for now I've removed pydeck, pyecharts, idom, xlsxwriter and pyarrow. You could readd pyarrow since that is available.
Co-authored-by: James A. Bednar <jbednar@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #2198 +/- ##
==========================================
+ Coverage 83.64% 84.02% +0.38%
==========================================
Files 183 181 -2
Lines 21947 21940 -7
==========================================
+ Hits 18357 18436 +79
+ Misses 3590 3504 -86
Continue to review full report at Codecov.
|
There is one fundamental thing to consider. Which branch should be promoted and shared with users? In principle any branch will be available on Binder. But it's important to agree on one that is promoted. My recommendation would be to promote the |
There are a few errors but they are not preventing the .ts from being built and don't cause any real issues. However I should add a CI check to ensure there are zero TS errors or warnings. Not entirely excited about a binder branch since that's an additional release step but given all the alternatives that still seems like the best approach. |
The alternative is just using the |
Sound to me like it ought to be using a tag indicating the latest release, rather than a separate branch? Our release workflow would then need to update |
Thanks again for this extraordinary effort @MarcSkovMadsen. I'll be adding binder links to all pages in a separate PR. |
Address #1500, #1369 (comment) and #2218.
This will enable opening the Panel examples on Binder. Whether it will also be possible to add a binder link to all examples is unknown and "nice to have" for this PR.
For now you can access binder via https://mybinder.org/v2/gh/holoviz/panel/binder?urlpath=lab/tree/examples
I would like a suggestion from @philippjfr or @jbednar which link we should provide when released. I see the following options
binder
branch.binder
branchmaster
branch.